Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: HELP: Error: The terraform-provider-mongodbatlas_v1.11.0 plugin crashed #1419

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

andreaangiolillo
Copy link
Collaborator

@andreaangiolillo andreaangiolillo commented Aug 28, 2023

Description

This PR fixes an NPE when the matcher & metrics threshold are provided with null fields

Link to any related issue(s): #1396

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

Next Steps:

  • Reproduce the issue with the resource migrated to the new framework
  • Backport the fix in 1.12.0 if needed

@andreaangiolillo andreaangiolillo changed the title fix: INTMDB-1017 fix: HELP: Error: The terraform-provider-mongodbatlas_v1.11.0 plugin crashed Aug 28, 2023
@andreaangiolillo andreaangiolillo marked this pull request as ready for review August 28, 2023 09:30
@andreaangiolillo andreaangiolillo requested a review from a team as a code owner August 28, 2023 09:30
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@AgustinBettati AgustinBettati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, agree with the next steps 👍

@andreaangiolillo andreaangiolillo merged commit 623dcdc into master Aug 28, 2023
21 checks passed
@andreaangiolillo andreaangiolillo deleted the INTMDB-1017 branch August 28, 2023 10:01
@@ -515,8 +514,11 @@ func expandAlertConfigurationMatchers(d *schema.ResourceData) []matlas.Matcher {

if m, ok := d.GetOk("matcher"); ok {
for _, value := range m.([]interface{}) {
v := value.(map[string]interface{})
if value == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we do something similar to

			if ok, v := value.(map[string]interface{}); !ok {
				break
			}

if vL[0] == nil {
return nil
}

v := vL[0].(map[string]interface{})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as here. I have not fully tried myself, but was wondering if something like ok, v := value.(map[string]interface{}); !ok would catch the impossibility to cast due to nil.

I also don't fully understand this one: in which cases would there be an array of length > 0 whose first value is nil? And why do we assume that all other values in the array will also be nil?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tried to do it but the ok was still true but I am not 100% sure now 🤦 . I will add an update in a few minutes to see if this would work. Thanks for flagging this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, We can use the ok,v pattern here. I opened #1420 to address this comment. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants